-
Couldn't load subscription status.
- Fork 118
feat: Accept nullable values in "tags" HashMap in Add action
#1395
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: Accept nullable values in "tags" HashMap in Add action
#1395
Conversation
Add action
|
Please take a look at this PR: @nicklan, @zachschuermann, and @scovich (I don't have the rights to add you as reviewers). |
|
It seems like we need some unit tests for this change? Negative tests can be tricky (maybe best avoided), but we should at least have positive tests that the newly allowed types and schemas are actually allowed and behaving as expected? |
eac45aa to
0e80a85
Compare
96639ad to
f3d0287
Compare
|
@scovich I uploaded a completely new version that doesn't modify derive-macros at all because I realized the initial approach was incorrect. We need to read NULL values and keep them in "tags," but with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new version of the change looks reasonable... but we will also need to update the row visitors because they don't support nullable map keys today.
|
@scovich So few more findings after your comment.
That's right, but we don't read tags at all: https://github.com/delta-io/delta-kernel-rs/blob/main/kernel/src/actions/visitors.rs#L120. Second thing. I also realized that other actions also have "tags":
|
| } | ||
|
|
||
| #[test] | ||
| fn test_add_tags_deserialization() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I would separate these cases into 3 unit tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll do it
|
@scovich Hey Ryan, can you please take a look at my response above, please: #1395 (comment)? If I'm missing something, or if changing the tags' schema doesn't make sense without supporting them in RowVisitor, I can also try adding parsing tags into RowVisitor. I just don't want to merge everything into a single PR if it can be two separate parts, but if you think it's better to combine both parts into one PR, I will do that! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What changes are proposed in this pull request?
Align the schema of AddAction with the schema in Scala implementation of Delta: https://github.com/delta-io/delta/blob/11873fe40f6fe7660b63b2c7611df40330865620/kernel/kernel-api/src/main/java/io/delta/kernel/internal/actions/AddFile.java#L59-L61
To achieve this, we need to support
Option<HashMap<K, V>>inallow_null_container_valuesmacros.How was this change tested?
We had a test where we check the expected types. I changed the expected value for tags' values from "required" to "nullable".